Refactor CI msrv & minver checks #2238
Conversation
CAD97
force-pushed
the
actions-rs-rip
branch
2 times, most recently
from
6824022
to
452e45d
Compare
There was a problem hiding this comment.
one question about this change, just to make sure I understand the motivation here.
as a side note, i'm not super excited about how all these files were re-formatted, i'm now concerned that any subsequent modifications i make will reformat them differently...perhaps we ought to check in an .editorconfig for yaml files or something, to ensure different people's editors don't reformat them as often. but, it's not a huge deal.
.github/workflows/CI.yml
Outdated
| minimal-versions: | ||
| # Check for minimal-versions errors where a dependency is too | ||
| # underconstrained to build on the minimal supported version of all | ||
| # dependencies in the dependency graph. | ||
| name: cargo check (-Zminimal-versions) | ||
| needs: check | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v3 | ||
| - uses: actions-rs/toolchain@v1 | ||
| - uses: dtolnay/rust-toolchain@stable | ||
| with: | ||
| toolchain: nightly | ||
| profile: minimal | ||
| override: true | ||
| - name: install cargo-hack | ||
| uses: taiki-e/install-action@cargo-hack | ||
| - name: "check --all-features -Z minimal-versions" | ||
| run: | | ||
| # Remove dev-dependencies from Cargo.toml to prevent the next `cargo update` | ||
| # from determining minimal versions based on dev-dependencies. | ||
| cargo hack --remove-dev-deps --workspace | ||
| # Update Cargo.lock to minimal version dependencies. | ||
| cargo update -Z minimal-versions | ||
| cargo hack check \ | ||
| --package tracing \ | ||
| --package tracing-core \ | ||
| --package tracing-subscriber \ | ||
| --all-features --ignore-private |
There was a problem hiding this comment.
what happened to the minimal-versions job? it seems to have disappeared --- is this because we think the +MSRV -Zminimal-versions run is sufficient to ensure minver compatibility? do we no longer need to check it on stable as well?
There was a problem hiding this comment.
Yes, the theory is that checking +MSRV -Zminimal-versions is sufficient, and that standard backwards-compatibility guarantees imply that +stable -Zminimal-versions will also work. This isn't guaranteed, but we can have fair confidence since we're testing stable with normal maximal version resolution.
|
Unfortunately the change here I think cannot be covered in an editorconfig; it's changing # from
---
key:
- list
- of
- values
# to
---
key:
- list
- of
- valuesand short of switching to flow syntax everywhere, there doesn't seem to be a standard1 "use this option" config file. Interestingly, in a quick search, nearly every example uses the second form except for the official YAML spec. Either way, we probably should normalize to one option or the other; most of our YAML is using shallow lists but not all. I went ahead and cleared that change out in a rebase to avoid making that call here. Footnotes
|
|
What this doesn't check is that That would necessitate another CI matrix split like in the existing feature powerset job. Given the Footnotes
|
|
Okay, the YAML change isn't really a big deal, and I'm fine with whatever style we end up using as long as my vscode install doesn't automatically reformat everything as soon as I open a config. :) |
|
I added the matrix jobs, it's your call whether to keep them or not. previous ci ran in 12:30; this ci ran in 14:00. A cursory overview suggests that each matrix job has about 20s of overhead compared to running that check in a shared job. (As far as CI time optimization goes, though, the big win would be caching dependency compilation on the test runners.) |
Hmm, the extra 1:30 or so of runtime is kind of a bummer, but I'm going to move forward with this change regardless, since it cleans up a bunch of stuff that I'd like to get fixed up. Let's try adding caching in a subsequent branch maybe? |
|
Sounds good to me. |
Motivation
Test msrv and minimal-versions compliance in the currently best-known fashion.
Solution